Conversation
ArtificialOwl
force-pushed
the
feat/noid/lexicon-entry-validating-value-on-set
branch
3 times, most recently
from
6d760bc to
a9488d7
Compare
ArtificialOwl
requested review from
icewind1991,
leftybournes,
nfebe and
provokateurin
and removed request for
a team
ArtificialOwl
force-pushed
the
feat/noid/lexicon-entry-validating-value-on-set
branch
from
a9488d7 to
8efae41
Compare
lib/private/AppConfig.php
Outdated
| * @param int $type value type {@see VALUE_STRING} {@see VALUE_INT} {@see VALUE_FLOAT} {@see VALUE_BOOL} {@see VALUE_ARRAY} | ||
| * | ||
| * @return bool TRUE if value was updated in database | ||
| * @throws UnacceptableValueException if lexicon does not validate value |
There was a problem hiding this comment.
This is a big change, you’r throwing a new Exception class.
It bubbles up to public API, no?
It should be noted on all calling methods, including in the public API.
And to not break existing code, ideally these exceptions should all inherit a common class so that calling code can be future-proof in case new ones get added later.
There was a problem hiding this comment.
the closure could throw that exception so that a message is added when facing a wrongly formatted value.
I can remove the exception and closure only returns true|false with no explaination of the issue
There was a problem hiding this comment.
I feel like this is worse, we get a silent failure now, it’s not even logged.
I’m not sure what would be better, either throw InvalidArgumentException as that’s already thrown by the same methods, or log an error to the log file.
I think it’s safer to simply log, that way when something breaks the instance is still usable, but the information is there in the log about something going wrong.
ArtificialOwl
force-pushed
the
feat/noid/lexicon-entry-validating-value-on-set
branch
2 times, most recently
from
c44bb57 to
d52b218
Compare
|
provokateurin
left a comment
provokateurin
left a comment
There was a problem hiding this comment.
LGTM, just one small oversight.
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
ArtificialOwl
force-pushed
the
feat/noid/lexicon-entry-validating-value-on-set
branch
from
edff996 to
1858698
Compare
|
It should log an error when validation fails. |
5 participants
Adding a new parameter to Lexicon
Entryso that aClosurecan be set to validate the config value before storing it to database.This would allow to use
provision_apiendpoints to set app/user configs while keeping an eye on its value without having a specific controller.would be nice to have this added to next RC of 33, as it could be used to confirm the format of a string when configuring Federated Teams